Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor commitPlacement to recursively insert nodes #17996

Merged
merged 2 commits into from
Feb 10, 2020
Merged

Refactor commitPlacement to recursively insert nodes #17996

merged 2 commits into from
Feb 10, 2020

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Feb 7, 2020 •
edited
Loading

This PR refactors the logice in commitPlacement so that we instead recursively traverse the nodes as we insert them. I noticed that commitPlacement was a hot function during the commit phase, and this should improve the runtime performance slightly, as we no longer need to mutate the fields on the fibers to point to different return pointers. Interestingly, this also improves the Flow coverage, which was any before, this has been refined and fixed.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 7, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 7, 2020 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 281a345:

Sandbox Source
epic-leakey-6kru4 Configuration

@sizebot
Copy link

sizebot commented Feb 7, 2020 •
edited
Loading

Details of bundled changes.

Comparing: 256d78d...281a345

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-dev.js +0.1% +0.1% 740.02 KB 740.85 KB 155.48 KB 155.61 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.1% +0.1% 749.41 KB 750.23 KB 157.68 KB 157.81 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 274.66 KB 274.77 KB 47.01 KB 47.03 KB RN_OSS_PROD
ReactFabric-dev.js +0.1% +0.1% 740.21 KB 741.03 KB 155.57 KB 155.69 KB RN_FB_DEV
ReactNativeRenderer-profiling.js 0.0% +0.1% 285.87 KB 285.98 KB 49.16 KB 49.22 KB RN_OSS_PROFILING
ReactFabric-prod.js 0.0% -0.0% 267.43 KB 267.43 KB 45.82 KB 45.82 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.1% +0.1% 749.58 KB 750.41 KB 157.78 KB 157.9 KB RN_FB_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 275.05 KB 275.16 KB 47.08 KB 47.1 KB RN_FB_PROD
ReactNativeRenderer-profiling.js 0.0% +0.1% 286.25 KB 286.36 KB 49.22 KB 49.28 KB RN_FB_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js 0.0% 0.0% 123.81 KB 123.87 KB 38.77 KB 38.77 KB NODE_PROFILING
ReactDOMServer-prod.js 0.0% 0.0% 48.99 KB 48.99 KB 11.18 KB 11.18 KB FB_WWW_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.88 KB 3.88 KB 1.55 KB 1.55 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 711 B 712 B UMD_PROD
react-dom-testing.profiling.min.js 0.0% -0.0% 121.37 KB 121.42 KB 38.36 KB 38.35 KB NODE_PROFILING
ReactDOMTesting-prod.js 🔺+0.1% 🔺+0.1% 340.91 KB 341.24 KB 62.23 KB 62.28 KB FB_WWW_PROD
ReactDOMTesting-profiling.js +0.1% +0.1% 351.69 KB 352.03 KB 64.18 KB 64.22 KB FB_WWW_PROFILING
react-dom-server.browser.production.min.js 0.0% -0.0% 20.46 KB 20.46 KB 7.5 KB 7.5 KB UMD_PROD
react-dom-test-utils.development.js 0.0% -0.0% 53.66 KB 53.66 KB 15.27 KB 15.27 KB NODE_DEV
react-dom-server.browser.development.js 0.0% 0.0% 135.26 KB 135.26 KB 35.92 KB 35.92 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.99 KB 10.99 KB 4.1 KB 4.1 KB NODE_PROD
react-dom.development.js +0.1% +0.1% 965.13 KB 965.96 KB 217.07 KB 217.19 KB UMD_DEV
react-dom.production.min.js 0.0% -0.1% 119.94 KB 120 KB 38.42 KB 38.39 KB UMD_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.25 KB 10.25 KB 3.47 KB 3.47 KB UMD_PROD
react-dom.profiling.min.js 0.0% 0.0% 123.58 KB 123.63 KB 39.57 KB 39.58 KB UMD_PROFILING
react-dom.development.js +0.1% +0.1% 959.22 KB 960.04 KB 215.4 KB 215.52 KB NODE_DEV
react-dom.production.min.js 0.0% -0.0% 120.03 KB 120.09 KB 37.66 KB 37.65 KB NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 9.99 KB 9.99 KB 3.38 KB 3.38 KB NODE_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.71 KB 3.71 KB 1.5 KB 1.5 KB NODE_DEV
ReactDOM-dev.js +0.1% +0.1% 986.67 KB 987.5 KB 218.19 KB 218.31 KB FB_WWW_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 642 B 643 B NODE_PROD
ReactDOM-prod.js 🔺+0.1% 🔺+0.1% 393.59 KB 393.92 KB 71.93 KB 71.98 KB FB_WWW_PROD
react-dom-testing.development.js +0.1% +0.1% 962.55 KB 963.38 KB 216.09 KB 216.21 KB UMD_DEV
ReactDOM-profiling.js +0.1% +0.1% 404.84 KB 405.18 KB 74.09 KB 74.15 KB FB_WWW_PROFILING
react-dom-testing.production.min.js 0.0% 0.0% 117.6 KB 117.65 KB 37.9 KB 37.91 KB UMD_PROD
react-dom-testing.profiling.min.js 0.0% -0.0% 121.12 KB 121.17 KB 39.04 KB 39.04 KB UMD_PROFILING
react-dom-testing.development.js +0.1% +0.1% 956.64 KB 957.46 KB 214.4 KB 214.52 KB NODE_DEV
react-dom-server.node.development.js 0.0% 0.0% 136.37 KB 136.37 KB 36.14 KB 36.15 KB NODE_DEV
react-dom-testing.production.min.js 0.0% -0.0% 117.69 KB 117.75 KB 37.25 KB 37.24 KB NODE_PROD
ReactDOMTesting-dev.js +0.1% +0.1% 985.24 KB 986.07 KB 217.47 KB 217.59 KB FB_WWW_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 20.79 KB 20.79 KB 7.62 KB 7.62 KB NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.1% +0.1% 621.77 KB 622.59 KB 131.24 KB 131.36 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% -0.1% 72.06 KB 72.15 KB 21.98 KB 21.96 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 617.04 KB 617.86 KB 130.05 KB 130.17 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.3% 71.76 KB 71.93 KB 21.56 KB 21.63 KB NODE_PROD
ReactTestRenderer-dev.js +0.1% +0.1% 632.48 KB 633.31 KB 130.85 KB 130.97 KB FB_WWW_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.66 KB 11.66 KB 3.6 KB 3.6 KB UMD_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.development.js +0.1% +0.1% 610.97 KB 611.79 KB 127.71 KB 127.83 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% 0.0% 19.95 KB 19.95 KB 6.56 KB 6.56 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 73.66 KB 73.76 KB 21.76 KB 21.78 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.86 KB 2.86 KB 1.24 KB 1.24 KB NODE_PROD
react-reconciler.development.js +0.1% +0.1% 614.19 KB 615.01 KB 129.04 KB 129.16 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 76.42 KB 76.51 KB 22.41 KB 22.44 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 677.4 KB 678.22 KB 145.89 KB 146.01 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 0.0% 107.41 KB 107.54 KB 32.53 KB 32.53 KB UMD_PROD
react-art.development.js +0.1% +0.1% 608.07 KB 608.89 KB 128.46 KB 128.58 KB NODE_DEV
react-art.production.min.js 🔺+0.2% -0.1% 72.38 KB 72.5 KB 21.68 KB 21.66 KB NODE_PROD
ReactART-dev.js +0.1% +0.1% 620.8 KB 621.63 KB 128.33 KB 128.45 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.2% 🔺+0.2% 237.97 KB 238.39 KB 40.11 KB 40.2 KB FB_WWW_PROD

ReactDOM: size: 0.0%, gzip: 🔺+0.1%

Size changes (experimental)

Generated by 🚫 dangerJS against 281a345

@sizebot
Copy link

sizebot commented Feb 7, 2020 •
edited
Loading

Details of bundled changes.

Comparing: 256d78d...281a345

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.1% +0.1% 608.05 KB 608.87 KB 128.46 KB 128.58 KB NODE_DEV
react-art.production.min.js 🔺+0.2% -0.1% 69.86 KB 69.98 KB 21.01 KB 20.99 KB NODE_PROD
react-art.development.js +0.1% +0.1% 677.38 KB 678.2 KB 145.88 KB 146 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 0.0% 104.86 KB 104.98 KB 31.83 KB 31.84 KB UMD_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.1% +0.1% 749.39 KB 750.22 KB 157.68 KB 157.8 KB RN_OSS_DEV
ReactFabric-dev.js +0.1% +0.1% 740.01 KB 740.84 KB 155.48 KB 155.6 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 0.0% 274.65 KB 274.76 KB 47 KB 47.03 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% +0.1% 285.85 KB 285.96 KB 49.15 KB 49.21 KB RN_OSS_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-test-utils.development.js 0.0% -0.0% 53.65 KB 53.65 KB 15.27 KB 15.27 KB NODE_DEV
react-dom.production.min.js 0.0% -0.0% 116.03 KB 116.08 KB 37.29 KB 37.28 KB UMD_PROD
react-dom-testing.development.js +0.1% +0.1% 956.61 KB 957.44 KB 214.39 KB 214.51 KB NODE_DEV
react-dom.profiling.min.js 0.0% 0.0% 119.55 KB 119.61 KB 38.45 KB 38.45 KB UMD_PROFILING
react-dom-testing.production.min.js 0.0% -0.0% 117.16 KB 117.22 KB 37.11 KB 37.09 KB NODE_PROD
react-dom.development.js +0.1% +0.1% 959.19 KB 960.02 KB 215.39 KB 215.5 KB NODE_DEV
react-dom-testing.profiling.min.js 0.0% -0.0% 120.84 KB 120.9 KB 38.21 KB 38.21 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.87 KB 3.87 KB 1.54 KB 1.54 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 116.09 KB 116.15 KB 36.63 KB 36.61 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.2 KB 1.2 KB 703 B 704 B UMD_PROD
react-dom.profiling.min.js 0.0% -0.0% 119.76 KB 119.82 KB 37.74 KB 37.74 KB NODE_PROFILING
react-dom-server.browser.development.js 0.0% -0.0% 139.3 KB 139.3 KB 36.94 KB 36.94 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20 KB 20 KB 7.4 KB 7.4 KB UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% -0.1% 3.7 KB 3.7 KB 1.5 KB 1.49 KB NODE_DEV
react-dom-server.browser.development.js 0.0% 0.0% 135.23 KB 135.23 KB 35.91 KB 35.91 KB NODE_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 19.93 KB 19.93 KB 7.39 KB 7.39 KB NODE_PROD
react-dom-test-utils.production.min.js 0.0% 🔺+0.1% 10.97 KB 10.97 KB 4.09 KB 4.1 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.92 KB 60.92 KB 16.07 KB 16.07 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 10.24 KB 10.24 KB 3.46 KB 3.46 KB UMD_PROD
react-dom-server.node.development.js 0.0% 0.0% 136.34 KB 136.34 KB 36.14 KB 36.14 KB NODE_DEV
react-dom-testing.development.js +0.1% +0.1% 962.53 KB 963.35 KB 216.08 KB 216.2 KB UMD_DEV
react-dom-testing.production.min.js 0.0% 0.0% 117.06 KB 117.12 KB 37.75 KB 37.75 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% -0.0% 60.62 KB 60.62 KB 15.99 KB 15.99 KB NODE_DEV
react-dom.development.js +0.1% +0.1% 965.11 KB 965.93 KB 217.05 KB 217.17 KB UMD_DEV
react-dom-testing.profiling.min.js 0.0% -0.1% 120.59 KB 120.64 KB 38.93 KB 38.91 KB UMD_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% -0.0% 9.98 KB 9.98 KB 3.37 KB 3.37 KB NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-reflection.development.js 0.0% 0.0% 19.93 KB 19.93 KB 6.56 KB 6.56 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% -0.1% 2.85 KB 2.85 KB 1.24 KB 1.24 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% +0.1% 610.95 KB 611.78 KB 127.7 KB 127.82 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 73.65 KB 73.75 KB 21.75 KB 21.78 KB NODE_PROD
react-reconciler.development.js +0.1% +0.1% 614.18 KB 615 KB 129.04 KB 129.16 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 73.64 KB 73.73 KB 21.74 KB 21.77 KB NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.64 KB 11.64 KB 3.59 KB 3.59 KB UMD_PROD
react-test-renderer-shallow.production.min.js 0.0% -0.1% 11.78 KB 11.78 KB 3.7 KB 3.7 KB NODE_PROD
react-test-renderer.development.js +0.1% +0.1% 621.75 KB 622.57 KB 131.23 KB 131.34 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% -0.1% 72.03 KB 72.13 KB 21.96 KB 21.94 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 617.01 KB 617.83 KB 130.04 KB 130.15 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.2% 🔺+0.3% 71.74 KB 71.91 KB 21.54 KB 21.61 KB NODE_PROD

ReactDOM: size: 0.0%, gzip: -0.0%

Size changes (stable)

Generated by 🚫 dangerJS against 281a345

@trueadm trueadm force-pushed the recursive-placement-node branch from 5dd6086 to 513c18b Compare February 7, 2020 11:16
@trueadm
Address feedback
281a345 
Other file too
@trueadm trueadm force-pushed the recursive-placement-node branch from d14fcde to 281a345 Compare February 8, 2020 10:11
acdlite
acdlite approved these changes Feb 10, 2020
View reviewed changes
Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay thanks

@trueadm trueadm merged commit df5fadd into facebook:master Feb 10, 2020
@trueadm trueadm deleted the recursive-placement-node branch February 10, 2020 23:38
gaearon added a commit to gaearon/react that referenced this pull request Apr 7, 2020
@gaearon
Revert "Refactor commitPlacement to recursively insert nodes (faceboo…
f9e55c3 
…k#17996)"

This reverts commit df5fadd.
gaearon added a commit that referenced this pull request Apr 7, 2020
@gaearon
Revert "Refactor commitPlacement to recursively insert nodes (#17996)" (
e69ca31 
#18517)

This reverts commit df5fadd.
gaearon added a commit that referenced this pull request Apr 8, 2020
@gaearon
Revert "Revert "Refactor commitPlacement to recursively insert nodes (#…
9bc6431 
…17996)" (#18517)"

This reverts commit e69ca31.
gaearon added a commit that referenced this pull request Apr 8, 2020
@gaearon
Revert "Revert "Refactor commitPlacement to recursively insert nodes (#…
b225d4f 
…17996)" (#18517)" (#18540)

This reverts commit e69ca31.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acdlite acdlite acdlite approved these changes

@sebmarkbage sebmarkbage Awaiting requested review from sebmarkbage

@gaearon gaearon Awaiting requested review from gaearon

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trueadm @sizebot @acdlite @facebook-github-bot